-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen.send_signal. #20010
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this PR will be accepted, but I left some comments.
Also, this PR should probably have a unit test.
Misc/NEWS.d/next/Library/2020-05-08-21-30-54.bpo-40550.i7GWkb.rst
Outdated
Show resolved
Hide resolved
This fixes a race condition, there's no way to reliably have a unit test. The process needs to exit between the return code check and the action itself. |
I think you could write a unit test by synchronising the threads so the race condition occurs. You can mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @FFY00, I checked the test and it simulates the race condition successfully 👍
Try not to force-push during reviews thought as it makes it harder to see what has been updated since the last time one viewed that changes.
You can click on |
Signed-off-by: Filipe Laíns <lains@archlinux.org>
Sorry, after writing all that I messed up the git history 😕 You can still view the correct diff here: https://github.com/python/cpython/compare/49e7004bc963966bfb2f264d6022ef4477f21bcc..3d7dfbc1c19280dd19693d8d8e0574590c0c1bee |
@gpshead do you have a minute to review this? |
The ProcessLookupError already means errno == ESRCH.
….send_signal. (pythonGH-20010) send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks). Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit 01a202a) Co-authored-by: Filipe Laíns <lains@archlinux.org>
GH-23439 is a backport of this pull request to the 3.9 branch. |
Sorry, @FFY00 and @gpshead, I could not cleanly backport this to |
….send_signal. (GH-20010) send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks). Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit 01a202a) Co-authored-by: Filipe Laíns <lains@archlinux.org>
….send_signal. (pythonGH-20010) send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks). Co-authored-by: Gregory P. Smith <greg@krypto.org>
All the implementation details have been explained the bug, please check it out.
https://bugs.python.org/issue40550